Monomorphize calculate_dtor instead of using function pointers
#77755
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
jyn514
added
I-compiletime
jyn514
changed the title
calculate_dtor instead of using function pointers
|
The input closure is called in a loop for each relevant impl and is trivial for at least one plausibly hot call ( @bors try |
|
Awaiting bors try build completion |
|
⌛ Trying commit fc17e91e14701a76c404bc05e44807db91e7261a with merge ffec759ae9bbc4d6d2235ff40ade6723a85bc7cc... |
|
☀️ Try build successful - checks-actions, checks-azure |
|
Queued ffec759ae9bbc4d6d2235ff40ade6723a85bc7cc with parent 7bc5839, future comparison URL. |
|
Finished benchmarking try commit (ffec759ae9bbc4d6d2235ff40ade6723a85bc7cc): comparison url. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
bugadani
force-pushed
the
perf-calc-dtor
branch
from
fc17e91
to
0d27b76
Compare
bugadani
changed the title
calculate_dtor instead of using function pointerscalculate_dtor instead of using function pointers
|
@ecstatic-morse could you please take a look at the results? Do you think this change would make sense? Thanks! |
|
📌 Commit 0d27b76 has been approved by |
bors
added
S-waiting-on-bors
…-morse Monomorphize `calculate_dtor` instead of using function pointers Change `calculate_dtor` to avoid dynamic dispatching. This change allows the empty functions to be optimized away. Based on the discussion in rust-lang#77754 (comment), the performance impact of this change was measured. Perf run results: https://perf.rust-lang.org/compare.html?start=7bc5839e99411aad9061a632b62075d1346cbb3b&end=ffec759ae9bbc4d6d2235ff40ade6723a85bc7cc
|
⌛ Testing commit 0d27b76 with merge a6aa039311c05d0f43e07f49a770cdac914daf0e... |
|
💔 Test failed - checks-azure |
bors
added
S-waiting-on-review
|
Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Seems like a spurious failure. @bors retry |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions, checks-azure |
|
This ended up as a 1% regression on the deeply-nested-async benchmark -- do we think that's likely to be spurious? The original perf results don't show this regression. Wall times also seem up across the board, but that could be noise. I am somewhat surprised by these results -- this PR looks like it shouldn't cause such significant effects. That said, it does seem like the extra inlining here could end up harmful in the end perhaps? I am inclined to revert it (or at least benchmark a revert). |
|
There is always a chance that this screwed up cache alignment, in combination with another PR that interacts with it. I wonder if a PR between the perf run and the merge pushed something out of alignment, maybe? If that is possible, I think this PR could cause unnecessary noise and should be reverted. But it would be nice to know for sure, what happened. |
Change
calculate_dtorto avoid dynamic dispatching. This change allows the empty functions to be optimized away.Based on the discussion in #77754 (comment), the performance impact of this change was measured.
Perf run results: https://perf.rust-lang.org/compare.html?start=7bc5839e99411aad9061a632b62075d1346cbb3b&end=ffec759ae9bbc4d6d2235ff40ade6723a85bc7cc